Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor account deletion in TunnelManager #5520

Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Nov 29, 2023

Currently TunnelManager.deleteAccount() is somewhat fragmented. DeleteAccountOperation only runs a network request that deletes account from backend, but it leaves the part for removing the tunnel and logging out device on completionHandler.

Completion handler executes on main queue however it taps into private unsetTunnelConfiguration() which returns control back on private queue. Subsequent call back to UI continues on the same private queue which violates UI threading model (all UI calls must happen on main queue).

This PR removes DeleteAccountOperation altogether and adds a new .delete action to SetAccountOperation, thereby consolidating account creation and deletion into one operation.


This change is Reviewable

Copy link

linear bot commented Nov 29, 2023

IOS-310 Refactor account deletion in TunnelManager

Currently TunnelManager.deleteAccount() is somewhat fragmented.

DeleteAccountOperation that it schedules only runs a network request that deletes account from backend, but it leaves the part for removing the tunnel and logging out device on completionHandler.

Completion handler executes on main queue however it taps into private unsetTunnelConfiguratin() which returns control back on private queue. Subsequent call back to UI continues on the same private queue which violates UI threading model (all UI calls must happen on main queue)

We should perhaps look into consolidating the entire "deletion" logic inside of DeleteAccountOperation and keep completion handler as simple as the following snippet:

func deleteAccount(
    accountNumber: String,
    completion: ((Error?) -> Void)? = nil // 1
) -> Cancellable {
    let operation = DeleteAccountOperation(/* ... */)
    operation.completionQueue = .main // 2
    operation.completionHandler = { [weak self] result in
        completion?(result.error) // 3
    }

Note that operation will ensure that completion is dispatched on main queue.

Also: I suggest to look at how SetAccountOperation works with .unset action. Maybe we could reuse it to provide additional .delete action (for ex. SetAccountOperation.unsetDeviceState() does very much the same thing as unsetTunnelConfiguration() from TunnelManager).

If you decide to consolidation account deletion + tunnel removal into DeleteOperation then don't forget to add the following exclusivity category for the operation to make sure nothing else attempts to mutate system VPN configuration while operation is running:

operation.addCondition(MutuallyExclusive(category: OperationCategory.manageTunnel.category))

Side note on current architecture of tunnel manager

  1. For the most part TunnelManager is a collection of operations that do their individual tasks. The manager sets dependencies between operations depending on what they do: mutate device state, settings, or touch system VPN configuration or any permutations of that.

  2. Some shared functionality that those operations often tap into is consolidated in TunnelInteractorProxy (private type) and injected via init() to each individual operation.

    This is basically a front for TunnelManager that hides private functions from the outside world but exposes it to operations via abstract protocol TunnelInteractor that private TunnelInteractorProxy implements.

    Example how to instantiate interactor (from within TunnelManager):

    TunnelInteractorProxy(self)
    
  3. Functions exposed by TunnelInteractor all rely on nslock (NSLock) to deal with races because some of them tap into public props like deviceState or settings which are often touched by other parts of the app.

  4. Operations use serial internalQueue to run internal work but also as a general rule of thumb, they tend to always switch back to that queue if some of sys calls come back on background threads. This design makes it a bit easier to debug and understand what's going on with the tunnel manager state, although not the most elegant given that we also have lock in use. Obviously all that can be gradually improved down the road.

@rablador rablador force-pushed the refactor-account-deletion-in-tunnelmanager-ios-310 branch from 36d7a78 to 47e4976 Compare November 29, 2023 15:25
@rablador rablador added the iOS Issues related to iOS label Nov 29, 2023
@rablador rablador requested a review from mojganii November 29, 2023 15:26
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one !

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadVPN/TunnelManager/SetAccountOperation.swift line 164 at r1 (raw file):

    ) {
        deleteAccount(accountNumber: accountNumber) { [self] result in
            interactor.setSettings(LatestTunnelSettings(), persist: true)

You should consider erasing the last used account number since it's no longer exists but of course you'd have to check the result before you can make that assumption. i.e if result.isSuccess { /* remove last used account number */ }

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)


ios/MullvadVPN/TunnelManager/SetAccountOperation.swift line 164 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

You should consider erasing the last used account number since it's no longer exists but of course you'd have to check the result before you can make that assumption. i.e if result.isSuccess { /* remove last used account number */ }

This is currently handled in TunnelManager.deleteAccount, but I'll move it into startDeleteAccountFlow instead.

@rablador rablador force-pushed the refactor-account-deletion-in-tunnelmanager-ios-310 branch 2 times, most recently from 94a925c to c1b96c8 Compare December 4, 2023 08:59
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r2.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @pronebird and @rablador)


ios/MullvadVPN/TunnelManager/TunnelManager.swift line 444 at r2 (raw file):

        completion: ((Error?) -> Void)? = nil
    ) {
        setAccount(action: .delete(accountNumber)) { [weak self] result in

⚠️ Variable 'self' was written to, but never read

@buggmagnet buggmagnet force-pushed the refactor-account-deletion-in-tunnelmanager-ios-310 branch from c1b96c8 to 7963de8 Compare December 4, 2023 09:16
@buggmagnet buggmagnet merged commit f929009 into main Dec 4, 2023
4 of 5 checks passed
@buggmagnet buggmagnet deleted the refactor-account-deletion-in-tunnelmanager-ios-310 branch December 4, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants